fix(parser): preserve strikethrough and other non-enumerated styles - #18

Fixes #17.

The reported bug was real and is fixed. Verifying it properly meant running the corpus suite that was supposed to cover this class of regression — which turned out to be inert, and which then surfaced two more parser bugs of the same family. Four commits, each independently reviewable.

1. fix(parser): strikethrough-only cells keep their style

_extract_style() decided whether a cell was styled from a hand-written attribute list that omitted font.strikethrough and font.underline, so a cell whose only formatting was one of those had its entire CellStyle discarded — taking the correctly-parsed font with it.

Rather than adding the two missing attributes as the issue suggested, has_style now compares the parsed font against a default FontStyle() sentinel. Each _extract_* helper already returns None when nothing is set, so the enumeration was redundant as well as lossy. That closes the whole class of bug: it also recovers fills with only bg_color and alignments with only text_rotation/indent, which the old list dropped too.

The secondary request is included — font_strikethrough is now exposed on chunk cells in to_json(), alongside font_color/fill_color.

Reproducer from the issue now prints:

internal B1 Strike has_style= True font= {'bold': False, 'italic': False, 'strikethrough': True}
json_cell {"address": "B1", ..., "font_strikethrough": true}

2. fix(parser): merge invariants hold when regions overlap

Found by this work, not reported. Excel forbids overlapping merged regions, but malformed and machine-generated files contain them, and _build_merge_lookup mapped every cell of every region into one flat dict. A cell in an intersection was written twice and the last region silently won, leaving the parse output self-contradictory two ways:

• the loser's master became a slave whose master was never flagged, so every other cell in its region pointed at an unflagged master (M2)
• a cell that was master of one region and slave of another came back flagged both (M4) and slave-with-no-master (M1), because CellParser pre-flags any openpyxl MergedCell as a slave

Overlaps are now resolved up front: regions are accepted first-come in reading order, any that intersect an accepted one is dropped, and each drop is reported as a WARNING instead of being lost silently. Reading order makes the survivor independent of the order the file lists ranges in, so the workbook hash stays deterministic; surviving regions keep their original ordering, so well-formed workbooks are bit-identical.

The merge lookup is now also the single authority for the flags — it sets master/slave exclusively and clears CellParser's provisional slave flag for merges we don't track, which additionally covers stale ranges openpyxl reports outside any declared region.

6 of 112 real workbooks were affected. Confirmed pre-existing by running check_invariants against the parser at 4834382: identical violation counts (2, 4, 13, 2, 2, 16) before and after, all now zero.

3. fix(parser): formatting on valueless cells is preserved

Found by this work, not reported. The same enumeration blind spot as commit 1, in the one place where a false negative discards data outright — and compounded by a second defect that hid it.

_has_meaningful_style() checked three font attributes, so it missed strikethrough, underline, font size and font name. It was also wrong in the opposite direction: building a partial Font leaves color as None while the inherited default font has one, so an explicitly formatted cell looked less styled than an untouched neighbour, and every untouched cell reported as styled.

It now asks openpyxl rather than re-deriving. A cell's format is recorded as non-zero style ids for exactly the aspects that differ from the workbook default — precise, and immune to growing another blind spot as style support widens. Validated against one cell per aspect plus an untouched control: correct on all ten, where comparing resolved style objects against fresh defaults is not (a loaded default font does not equal Font()).

The predicate was moot either way: it spared a valueless cell from the early continue, then the gate that stores cells dropped anything is_empty. Styling on empty cells was parsed and then thrown away. That gate now keeps a cell carrying a style — strikethrough or a fill on an otherwise blank row is precisely the "deprecated / void" marker that motivated this issue, and dropping it silently is the same data loss one layer down.

Correcting the predicate also removed an accident the old one was holding up, and this is the part worth reviewing closely. A merge master arrives as an ordinary Cell, not a MergedCell, so the empty-cell skip never exempted it — an empty master survived only because the broken predicate called every untouched cell styled. Reporting those cells honestly started dropping empty masters, stranding every slave in the region with a merge_master that no longer existed: 348 M2 violations across 44 corpus workbooks, none of which the default suite could see. The skip now spares any cell taking part in a merge, recognising the master via the merge lookup.

Costs 3.6% more cells across the 192-workbook corpus (9,013 on 251,376). Cells kept this way still report is_empty, so nothing mistakes them for data.

4. test(corpus): make the corpus suite actually exercise the parser

Every corpus source was dead, and the download tests asserted only isinstance(files, list) while download_and_extract_xlsx swallowed RequestException into a log warning. They reported green having fetched nothing:

So tests/fixtures/corpus/ stayed empty, and because corpus_files is computed at import time, the eight robustness tests depending on it skipped silently. The suite spent 5.5 minutes verifying nothing.

Repointed at sources chosen for durability over volume: sdists of Excel-handling PyPI packages (published artifacts are immutable, so they can't rot) and upstream parser fixture directories enumerated through the GitHub contents API, so an upstream rename costs one file rather than the whole corpus. 80 files in 7.5s, down from 0 in 325s.

The tests now assert on what arrived. A genuinely offline machine skips; a reachable source returning nothing usable fails. Non-container members are skipped at extraction — upstream suites ship a zero-byte file and an encrypted OLE workbook under an .xlsx name.

Two contracts the corpus was conflating are now split. Upstream fixture suites include files that are valid ZIPs but not valid OOXML packages (missing [Content_Types].xml, missing sharedStrings.xml, a style attribute openpyxl rejects). openpyxl cannot open them either, so test_has_sheets was asserting the impossible; it skips them now, and test_unloadable_files_report_an_error covers the contract that does apply — degrade with a recorded ERROR rather than crash or come back silently empty. test_success_rate's 95% threshold likewise measures the loadable subset, so it tracks this parser's health rather than how many negative fixtures upstream happens to ship.

Verification

• Default suite: 1249 passed (was 1181 before this branch)
• Corpus suite, 192 workbooks (80 downloaded + 112 local bench files): 1157 passed, 192 skipped, 0 failed
• Ruff: no new findings on changed files; repo total unchanged at the pre-existing 49
• mypy: no new findings in changed code; the 6 in sheet_parser.py are pre-existing (missing stubs, three unrelated spots)
• Cross-checked strikethrough against all 192 corpus workbooks with openpyxl as ground truth

Note

The ruff and mypy CI jobs are still non-gating (|| true), and the mypy job targets src/ks_xlsx_parser, which no longer exists after the excel_parser rename — so it currently checks nothing. Untouched here.

🤖 Generated with Claude Code